fix(runtime): distinguish usage limits from auth errors - #2521
Conversation
ec8c6f3 to
4ffeea9
Compare
4ffeea9 to
b1e26b6
Compare
6598eb0 to
083ba9b
Compare
Preserve the original apache#2521 implementation history while accepting apache#2675 and current main as the canonical Runtime Host diagnostic path. The follow-up commit reapplies only the remaining classification and presentation changes.
083ba9b to
ce27694
Compare
|
After #2675 landed, I went back through this branch and rebuilt it on the current #2675 fixed the diagnostic path itself: it preserves an allowlisted/redacted/bounded provider summary at ModelAdapter, records the terminal message in the canonical RuntimeEvent, carries it through Runtime Host failed-Turn snapshots and recovery, and attaches the existing execution trace to Desktop error reports. I dropped this PR's older parallel Runtime Host, Headless, continuity-schema, and diagnostics changes instead of resolving those conflicts in favor of duplicated code. The part still missing was the meaning assigned before that path runs. Current This revision keeps Kimi's generic I added the observed Kimi envelope as a provider-boundary fixture and a TUI projection test. Local verification passed: Runtime 3,316 tests (3 skipped), CLI 352 tests, Desktop typecheck/checks and the targeted 10-test matrix, plus repository lint. I did not repeat a live request against an exhausted Kimi account, so that remains the real-account verification boundary. 中文对照#2675 合并后,我重新检查了这个分支,并基于当前 #2675 已经解决的是“诊断如何安全传到界面”:ModelAdapter 会保留经过白名单筛选、脱敏和长度限制的 provider 摘要;terminal RuntimeEvent 记录 canonical 失败消息;Runtime Host 将它带入失败 Turn snapshot 和恢复流程;Desktop 复制错误报告时会附上已有的 execution trace。因此我删除了本 PR 旧版本中并行实现的 Runtime Host、Headless、continuity schema 和 diagnostics 改动,没有为了消除冲突而保留重复代码。 还缺的是这条链路之前的语义分类。当前 这一版不猜测 Kimi 的通用 我加入了实际观察到的 Kimi envelope fixture 和一项 TUI 投影测试。本地验证通过:Runtime 3,316 项(3 项跳过)、CLI 352 项、Desktop typecheck/checks 与 10 项针对性矩阵,以及全仓 lint。没有再次用一个已耗尽额度的真实 Kimi 账户发请求,因此真实账户端到端验证仍是当前边界。 |
ce27694 to
f675515
Compare
|
One concrete before/after for the Kimi Code Plan case: Before this PR, the observed After this PR, Kimi's generic This deliberately does not classify the failure as 中文对照Kimi Code Plan 这个真实场景的前后对比如下。 本 PR 之前,实测 本 PR 之后,Kimi 的通用 这里不会因为 message 中出现 “usage limit” 就把错误分类成 |
f675515 to
ae1d994
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
I reviewed exact head ae1d994dbd83a375773e4702310c7180ad9712e1 across provider-owned structured evidence, retry metadata, ModelAdapter projection, TUI/Desktop presentation, recovery actions, and connection-test boundaries. The stronger structured classification and account-state retry policy are useful, but two active Desktop surfaces still reclassify the exact Kimi shape incorrectly; see the P2 inline findings.
This is a cohesive classification-and-presentation slice with green checks, so I do not recommend splitting it or deleting the focused provider fixtures. A follow-up should also consider reusing this authority in connection testing, which still maps every 403 to needs_reauth, but I am not assigning that untouched scope a separate P-level finding here.
Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.
ae1d994 to
1fd42a9
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for tightening the provider classification and addressing the earlier Desktop permission-label issue. I reviewed the current head, including the Runtime Host projection and connection-test paths. Two remaining gaps appear to share the same root cause: the structured provider meaning is established in ModelAdapter, but it is not preserved through every consumer, so downstream paths infer it again from HTTP status or event shape.
[P2] Preserve the provider summary through the real Runtime Host projection
ModelAdapter emits the observed Kimi failure with code: "permission_error" and a bounded provider summary. However, the canonical failed TurnSnapshot stores only failureClass and failureMessage, and RuntimeHostSessionProjector reconstructs the error with reason: root.failureClass but no code.
The real Desktop path therefore receives reason: "permission_error" and no code. sessionEventErrorMessage() skips the new event.code branch and falls back to the generic conversation-error copy, so the provider explanation still does not reach Desktop. The new Desktop test passes because it constructs an event containing code directly, which the Runtime Host terminal projection does not produce.
Could we carry explicit provider-summary provenance through the Runtime Host snapshot and projection, then cover the full ModelAdapter → failed TurnSnapshot → RuntimeHostSessionProjector → Desktop path? Desktop should render the upstream message directly only when that boundary has explicitly marked it as a safe, bounded provider summary.
[P2] Remove the parallel status-only classifier from connection testing
The connection-test path still maps every HTTP 401/403 to auth, and the connection-effect coordinator persists that result as needs_reauth. Desktop Settings and the command-palette presentation also continue to treat a 403 as an authentication problem.
With the same Kimi envelope covered by this PR, a normal model turn is now neutral and non-retryable, while Settings → Test connection still tells the user to re-authenticate. Could connection testing consume the same provider classification authority, with a regression proving that this Kimi 403 does not become needs_reauth?
The smallest complete shape I see is one structured provider-failure result—semantic class, retryability, stable code, safe summary, and provenance—produced by the runtime owner and carried through Runtime Host and connection testing. Presentation layers can then render that result without maintaining additional status or text classifiers.
Separately, the current head conflicts with main in packages/cli/src/__tests__/pi-transcript.test.ts. The existing green checks ran against the older base, so the affected checks will need to run again after the conflict is resolved.
Focused verification on the reviewed head:
- Runtime classification and ModelAdapter tests: 36 passed.
- Desktop provider-failure presentation tests: 5 passed, although the test described above does not exercise the real Runtime Host projection.
- The earlier
permission_error→ local permission-label issue is fixed and is not being repeated here.
中文对照
感谢收紧 provider 错误分类,也感谢修复之前 Desktop 将 permission_error 显示为本地权限请求的问题。我沿着当前 head 检查了 Runtime Host 投影和连接测试路径。目前剩余的两个缺口有同一个根因:ModelAdapter 已经建立了结构化的 provider 语义,但这个语义没有贯穿所有消费方,导致下游仍然根据 HTTP 状态码或事件形状重新推断。
[P2] 在真实 Runtime Host 投影中保留 provider 说明
ModelAdapter 会把实测的 Kimi 错误转换为 code: "permission_error",并附带经过长度限制的安全 provider 摘要。但是,canonical failed TurnSnapshot 只保存 failureClass 和 failureMessage;RuntimeHostSessionProjector 重建错误事件时只写入 reason: root.failureClass,不会保留 code。
因此,真实 Desktop 路径收到的是 reason: "permission_error",而不是带有 code 的事件。sessionEventErrorMessage() 会跳过新增的 event.code 分支,最终回退到通用错误文案,provider 的具体说明仍然无法抵达 Desktop。新增 Desktop 测试之所以通过,是因为它手工构造了带 code 的事件,但 Runtime Host 的 terminal projection 实际不会产生这种形状。
是否可以让 Runtime Host snapshot 和 projection 显式携带 provider summary 的来源标记,并通过完整的 ModelAdapter → failed TurnSnapshot → RuntimeHostSessionProjector → Desktop 链路覆盖回归?Desktop 只应在上游明确标记消息为安全、限长的 provider summary 时直接展示它。
[P2] 删除连接测试中的平行状态码分类
连接测试路径仍然把所有 HTTP 401/403 映射为 auth,connection-effect coordinator 随后会把结果持久化为 needs_reauth。Desktop Settings 和命令面板也仍然把 403 显示为鉴权问题。
对于本 PR 已覆盖的同一个 Kimi 错误,正常模型调用现在会得到 neutral、non-retryable 的结果,但 Settings → Test connection 仍然会要求用户重新认证。连接测试是否可以消费同一个 provider classification authority,并增加一条回归测试,证明这个 Kimi 403 不会变成 needs_reauth?
我认为最小完整的终态是由 runtime owner 产生一个统一的结构化 provider failure result,其中包含语义类别、是否可重试、稳定 code、安全摘要和 provenance;Runtime Host 与连接测试都传递或消费这个结果。这样展示层不再需要维护额外的状态码或文本分类器。
另外,当前 head 与 main 在 packages/cli/src/__tests__/pi-transcript.test.ts 存在冲突。现有绿灯基于旧 base,解决冲突后需要重新运行受影响的检查。
针对 reviewed head 的 focused verification:
- Runtime classification 与 ModelAdapter:36 项通过。
- Desktop provider-failure presentation:5 项通过,但上面提到的测试没有覆盖真实 Runtime Host 投影。
- 之前的
permission_error→ 本地权限标签问题已经修复,这里不再重复提出。
AI-assisted review disclosure: Codex coordinated the source inspection and focused verification on exact head 1fd42a9bc3aca92214d9397efeeeaeac3b37bac9, with Claude Opus 5 providing an additional adversarial pass. The human reviewer examined the evidence, independently evaluated the findings, and owns the final review judgment.
1fd42a9 to
2b90ae3
Compare
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWhat this PR solvesThe PR distinguishes authentication, permission, billing, usage-limit, rate-limit, and availability failures. Structured provider identifiers take precedence over HTTP status codes. Bare HTTP 403 responses remain unknown and non-retryable. Existing bare-429 behavior remains unchanged. The change prevents Kimi Source of truth and solution scopeThe PR extends the shared provider-error classification and ModelAdapter path. It does not create a parallel diagnostic path.
The solution is the smallest coherent change supported by the diff. The added fields, protocol validation, presentation mappings, and tests preserve classification and bounded-message metadata across connection tests and failed turns. No safe deletion or simplification is evident without weakening regression coverage or message-safety validation. Validation and risksTests cover structured provider identifiers, nested causes, Kimi plan-limit responses, permission errors, usage limits, billing, authentication, rate limits, availability, retry behavior, bare 401/403 responses, bounded-message propagation, Runtime Host protocol validation, failed-turn projection, TUI, Desktop, CLI, and connection-test presentation. Reported verification passed for Runtime, Runtime Host, Desktop, typecheck, formatting, diff checks, and reverse-mutation checks. No live request was made against an exhausted Kimi account. Unrelated Computer Use test-harness flakes were reported in CI and tracked separately in issue Complexity deltaThe PR adds:
The PR removes:
The shared authority replaces distributed status-based decisions. The new branches and fields preserve required behavior across several surfaces. The tests increase maintenance work but protect classification precedence, retry behavior, protocol validation, and safe presentation. Total maintenance complexity stays justified. Review-relevant risks
Required checks remain unverified from the available evidence. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe change adds shared provider-failure classification. Runtime, Runtime Host, Desktop, and CLI preserve structured failure details, bounded provider messages, localized explanations, and recovery actions. ChangesProvider failure flow
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The PR improves provider-limit classification, but current failure paths can still misclassify some status-wrapped errors and replace localized CLI/Desktop guidance with less appropriate copy. Merge readiness remains moderate until these bounded correctness and presentation issues are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Provider
participant Runtime
participant RuntimeHost
participant Desktop
Provider->>Runtime: error response with status and provider code
Runtime->>Runtime: classify provider failure and bound message
Runtime->>RuntimeHost: failed turn or connection result
RuntimeHost->>Desktop: validated providerFailure metadata
Desktop->>Desktop: localize class or render bounded message
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
37713c9 to
622bc7e
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed exact head 622bc7e2ab3681b47280543b1343e3d3b9572ba6, including the full diff, green CI, previous reviews and replies, and the complete Runtime → Runtime Host → Desktop projection.
The problem definition is correct: structured provider identifiers are stronger evidence than ambiguous HTTP status codes, and account-state failures should not be retried as transient throttling. The earlier findings about preserving the bounded provider result through Runtime Host and removing the connection coordinator’s status-only classifier are fixed on this head.
Two P2 gaps remain, with the same root cause: provider failure meaning still has multiple authorities.
-
Desktop connection-test presentation still re-infers meaning from status and message text. A result with
statusCode: 403anderrorClass: "unknown"is still rendered as authentication failure by Settings and the command-palette path; reparsing the error message repeats the same mistake. The 429 path also returns before reading the structured response body, sousage_limit_reachedandinsufficient_quotaare collapsed into transient rate limiting. -
Durable provider diagnostics still let HTTP status override structured meaning.
permission_denied + 403becomesAuth,usage_limit_reached + 429becomesRateLimit, and the new classes fall toOtherwithout a status. That makes Runtime behavior and persisted execution telemetry disagree about the same failure.
The smallest complete fix is one structured provider-failure result—semantic class, retryability, status, stable code, bounded summary, and provenance—produced by the provider boundary and consumed without downstream reclassification. UI presentation and durable diagnostics should project that result rather than rebuilding taxonomy from HTTP/text.
I do not recommend splitting the PR. Once those remaining consumers use the same authority and focused regressions cover the real Settings and diagnostic paths, this should be ready.
Disclosure: Codex performed the read-only source, contract, lifecycle, test, CI, and prior-feedback analysis. The human contributor remains responsible for verifying the findings and the final merge decision.
中文
问题定义正确,但 provider failure 仍有多个 authority:Settings/命令面板继续把 neutral 403 显示成鉴权失败,429 跳过结构化额度信息;durable telemetry 又把 permission/usage 改写为 Auth/RateLimit。应由一个结构化结果贯穿所有消费者。
|
Addressed the latest provider-failure findings in b529c3f. A shared ProviderFailureResult is now the single structured authority from Runtime normalization through connection tests, Host protocol, durable telemetry, Settings, and the command palette. Structured provider meaning outranks HTTP fallback; neutral 403 is not relabeled as auth, usage_limit_reached + 429 remains UsageLimit, and only explicitly bounded provider summaries are displayed. Runtime, Runtime Host, and Desktop full suites pass sequentially; all-workspace typecheck, Biome, diff checks, and two reverse mutations pass. simplify-audit found no P0-P2 issues. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/runtime/src/__tests__/provider-error-classification.test.ts (1)
510-527: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test restates the
errorPresentationFromClassswitch.The four assertions duplicate the constant table in
provider-error-classification.tslines 704-730. It protects no observable behavior beyond the mapping itself, and it must be edited whenever the copy changes. The account-state coverage in the tests above already pins the classes.Disposition: optional. Delete this test, or fold the reason/message expectation into the surface test that renders it.
Based on path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
packages/runtime/src/__tests__/provider-conformance.test.ts (1)
617-638: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the bare-401 case to pin the message policy.
This test asserts only
statusCodeanderrorClass. It does not assertresult.errorMessageorresult.providerFailure, so it does not catch that an empty body now yields'Provider request failed (status=401)'marked as a bounded provider message.Disposition: optional, and it becomes useful once the bounded-message policy is settled. Add an assertion that a body-less failure carries no
boundedProviderMessage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bad536a-3edc-48b3-bc6d-178ed5ebbd12
📒 Files selected for processing (36)
apps/desktop/src/main/__tests__/provider-failure-presentation.test.tsapps/desktop/src/main/__tests__/runtime-host-connections-ipc-main.test.tsapps/desktop/src/main/runtime-host-connections-ipc-main.tsapps/desktop/src/renderer/app-shell-copy.tsapps/desktop/src/renderer/locales/conversation-copy.tsapps/desktop/src/renderer/model-connection-errors.tsapps/desktop/src/renderer/session-error-presentation.tsapps/desktop/src/renderer/session-status-presentation.tsapps/desktop/src/renderer/settings/provider-panel-shared.tspackages/cli/src/__tests__/pi-transcript.test.tspackages/core/package.jsonpackages/core/src/events.tspackages/core/src/llm-connections.tspackages/core/src/provider-failure.tspackages/core/src/runtime-event.tspackages/runtime-host/src/__tests__/canonical-session-projection.test.tspackages/runtime-host/src/__tests__/connection-effect-coordinator.test.tspackages/runtime-host/src/__tests__/connection-effects-protocol.test.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/session-projector.test.tspackages/runtime-host/src/adapter/session-projector.tspackages/runtime-host/src/protocol/connection-effects.tspackages/runtime-host/src/protocol/turn.tspackages/runtime-host/src/server/canonical-turn-snapshot.tspackages/runtime-host/src/server/connection-effect-coordinator.tspackages/runtime/src/__tests__/ai-sdk-flow.test.tspackages/runtime/src/__tests__/model-adapter.test.tspackages/runtime/src/__tests__/provider-conformance.test.tspackages/runtime/src/__tests__/provider-error-classification.test.tspackages/runtime/src/__tests__/provider-request-telemetry.test.tspackages/runtime/src/ai-sdk-flow.tspackages/runtime/src/connection-effect-outcome.tspackages/runtime/src/model-adapter.tspackages/runtime/src/model-protocol.tspackages/runtime/src/provider-error-classification.tspackages/runtime/src/test-connection.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the latest update. It fixes the earlier raw Kimi 403 exposure, routes adapter events through normalizeProviderFailure, and adds useful bounded-message coverage. One independent reviewer and a separate read-only DeepSeek V4 Flash high-effort pass both reproduced the cause-chain issue below; they also agreed that presentation should consume the Runtime-owned result without adding a second eligibility policy.
The problem definition is right: Runtime should be the single authority that turns provider/SDK failures into a safe, durable result. The current implementation is close, but providerFailureDiagnosticFacts() still selects one object from the cause chain instead of merging the strongest facts across it. A transport wrapper carrying HTTP 403 can therefore hide an inner structured usage_limit_reached, permission_denied, or context_length_exceeded code. That changes both the durable class and recovery behavior.
From first principles, collect a bounded evidence record across the cause chain, with structured provider semantics outranking wrapper transport status and status remaining a fallback. Then project one complete ProviderFailureResult—reason/class, retryability, bounded message, and optional code—and let CLI/Desktop render that contract directly. This removes the current duplicated presentation gates and is simpler than teaching each surface which subset of the result is safe.
I did not run a local test suite; this review is based on the latest head, focused static tracing, current green CI, one independent @reviewer pass, and a read-only ollama-cloud/deepseek-v4-flash high-effort review. AI-assisted review; I verified the reported paths and classifications.
中文评论
感谢更新。最新版本已经修复了此前 Kimi 403 原文暴露的问题,adapter event 也统一走 normalizeProviderFailure,并补了 bounded message 测试。
剩余核心问题是 cause chain 仍然只选择其中一个对象,而不是合并整条链上的最强证据。外层 403 transport wrapper 会遮住内层的 usage_limit_reached、permission_denied 或 context_length_exceeded,从而改变持久化分类和恢复策略。
更符合第一性原理的方案是:Runtime 在整条 cause chain 上生成一个有界 evidence record,结构化 provider 语义高于外层 HTTP 状态,状态只作为 fallback;随后投影完整的 ProviderFailureResult,CLI/Desktop 直接消费,不再各自增加安全显示资格判断。
本次未运行本地测试;结论来自最新 head 的静态追踪、当前绿色 CI、一个独立 reviewer 和一次只读的 DeepSeek V4 Flash high-effort 审查。AI 辅助审查;我已人工复核相关路径与分类结果。
f85faa3 to
94aac3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b583ce7-082b-4071-8fb3-e29b34fd144a
📒 Files selected for processing (17)
apps/desktop/src/main/__tests__/provider-failure-presentation.test.tsapps/desktop/src/renderer/model-connection-errors.tsapps/desktop/src/renderer/session-status-presentation.tspackages/cli/src/__tests__/pi-transcript.test.tspackages/cli/src/pi-transcript.tspackages/core/src/__tests__/runtime-event.test.tspackages/core/src/runtime-event.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/session-projector.test.tspackages/runtime/src/__tests__/model-adapter-onerror.test.tspackages/runtime/src/__tests__/model-adapter.test.tspackages/runtime/src/__tests__/provider-conformance.test.tspackages/runtime/src/__tests__/provider-error-classification.test.tspackages/runtime/src/model-adapter.tspackages/runtime/src/network/__tests__/scoped-fetch-transport.test.tspackages/runtime/src/provider-error-classification.tspackages/runtime/src/test-connection.ts
💤 Files with no reviewable changes (1)
- packages/runtime/src/tests/model-adapter-onerror.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/core/src/runtime-event.ts
- packages/runtime-host/src/tests/protocol.test.ts
- apps/desktop/src/renderer/session-status-presentation.ts
- packages/runtime-host/src/tests/session-projector.test.ts
- packages/runtime/src/tests/model-adapter.test.ts
- packages/runtime/src/tests/provider-conformance.test.ts
- apps/desktop/src/main/tests/provider-failure-presentation.test.ts
- packages/runtime/src/test-connection.ts
- packages/runtime/src/model-adapter.ts
- apps/desktop/src/renderer/model-connection-errors.ts
- packages/runtime/src/provider-error-classification.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
Astro-Han
left a comment
There was a problem hiding this comment.
The direction is sound: a single structured provider-failure authority is the right seam, and the UI consumers now preserve the more specific account and request semantics. I found two protocol-boundary defects that need to be resolved before this is safe to merge. The smallest solution is to bump the compatibility epoch for the new strict wire shape and make the producer obey the decoder's message-provenance invariant.
AI-assisted review disclosure: Codex verified the final findings against the current head, protocol decoders, producers, consumers, and focused tests. Two independent reviewer-agent passes and an OpenCode Go DeepSeek V4 Flash (high) adversarial pass were used as inputs; unsupported claims were discarded. No local tests were run.
中文复核
整体方向正确:把 provider failure 收敛为单一结构化权威是合适的扩展点,新的 UI consumer 也确实保留了更细的账户和请求语义。但当前有两个协议边界问题:新增严格 wire 字段却没有提升兼容 epoch;producer 还会生成 decoder 明确拒绝的 message provenance 组合。最小修复是提升 epoch,并让 producer 与 decoder 的 provenance 约束一致。
本次为 AI 辅助审查:Codex 在最新 head 上核验协议 decoder、producer、consumer 与相关测试;另使用两次独立 reviewer 审查及一次 OpenCode Go DeepSeek V4 Flash(high)对抗审查,未被代码支持的指控已剔除。未运行本地测试。
|
/agentic_review |
Code Review by Qodo
1.
|
Astro-Han
left a comment
There was a problem hiding this comment.
The error taxonomy is much stronger on this head: compatibility epoch, bounded-message transport, cause-chain aggregation, structured account states, neutral 403 handling, and Desktop/TUI presentation are all aligned.
I’m leaving COMMENT for one remaining precedence bug. The implementation comments say a provider's structured context-overflow code is unconditional evidence and is checked before the text layer, but the actual branch runs abort wording and numeric 402/429/401 first. That can turn a real context_length_exceeded envelope into Abort or RateLimit and bypass compaction/recovery. I left the focused P2 inline.
AI-assisted review disclosure: two Codex reviewers independently reviewed exact head f29054b; Codex resolved their differing conclusions by tracing the actual classification order.
中文说明
当前错误分类体系的大部分边界已经统一。剩余一个优先级 P2:代码注释声明结构化 context-overflow code 是强证据、应早于文本判断,但实际 abort 文本和 402/429/401 状态先返回,可能把真正的 context overflow 误分成 Abort/RateLimit,从而跳过压缩恢复。建议调整判断顺序并补组合 envelope 回归测试。
|
This PR changes user-visible provider error presentation in Desktop and the TUI. Could you please add screenshots of at least one affected usage-limit or provider-permission state, showing the revised message and recovery action? A compact Desktop and TUI composite is welcome. Please sanitize provider and account details. Thanks! Posted by Codex on behalf of Astro-Han. |
6f7cd5d to
2e5fe9c
Compare
|
Reopened: the head branch was briefly reset during a local tooling mishap earlier today, which appears to have triggered an automatic close. The branch has been fully restored (rebased onto current |
d62e979 to
86bb419
Compare
jackwener
left a comment
There was a problem hiding this comment.
Review performed by the assigned Kabi review orchestrator for the Qronos review task, bound to exact head db176470e08461a3efa6c30c40d2ac38db192673. Result: NO-GO with 2×P2; no approval was submitted. The exact-head test and audit check-runs are green, but this PR is currently CONFLICTING and must be rebased before merge.
| const providerRequestId = | ||
| firstProviderField(sources, ['requestId', 'request_id']) ?? | ||
| boundedProviderField(facts.responseHeaders?.['x-request-id']); | ||
| const retry = providerRetryMetadataFromFacts(facts); |
There was a problem hiding this comment.
[P2] Derive retryability from the normalized failure class/status. A provider envelope such as { error: { status: 500, message: "upstream authentication service temporarily unavailable" } } is accepted by the nested-status extraction above and returns errorClass: "ProviderUnavailable" with httpStatus: 500, but this call reclassifies the original weak authentication wording and returns retryable: false. The Runtime retry owner therefore skips a transient 5xx even though the same result says it is provider-unavailable. Existing tests cover top-level statusCode: 500, where numeric evidence wins earlier, so they do not catch this nested-provider shape. Please compute retry metadata from the same normalized errorClass/httpStatus authority used in the returned result, and add this nested-status regression.
There was a problem hiding this comment.
Confirmed by repro: providerFailureResult returned ProviderUnavailable/500 while providerRetryMetadata re-classified the weak wording and returned retryable: false. Retry now derives from one normalized classification (text-derived Auth is demoted when the envelope's own numeric status contradicts it, matching the failure projection) and resolves the numeric status through the same nested-envelope lookup. Regression matrix: 500+auth-text → retryable, 401 → not, 500+structured auth code → not (still protected), capacity → retryable.
| auth: 'Authentication failed', | ||
| provider_billing: 'Provider billing required', | ||
| provider_permission: 'Provider access denied', | ||
| provider_unavailable: 'Provider returned an error', |
There was a problem hiding this comment.
[P2] Add the Runtime-owned provider_capacity reason to this bilingual map. Runtime emits that stable reason for provider capacity/full-load failures, but this renderer has no entry for it. Because unbounded provider text is intentionally cleared before projection, a real notice with runtimeError.reason = "provider_capacity" and text = "" renders only The task run failed. Try again later. (and the equivalent generic Chinese fallback), erasing the actionable wait/switch-model guidance that Desktop already preserves. The existing transcript suite has no capacity case, so all 65 tests pass. Please map this closed-vocabulary reason in both locales and add a projection/render regression.
There was a problem hiding this comment.
Confirmed by repro: an empty-text notice with reason = "provider_capacity" fell through to the generic fallback in both locales. Added the bilingual entries plus a regression test asserting the capacity guidance renders and the generic fallback does not.
Keep ambiguous 403 responses out of authentication classification, preserve the bounded provider explanation added by apache#2675, and use structured provider identifiers for stable account-state meanings. Generated-by: Maka
Generated-by: Maka
Mark ModelFailure messages taken from the provider-failure summary as bounded, display-safe provider wording and carry the marker through the durable error content so presentation layers never render unbounded transport text. Connection testing previously kept a parallel status-only classifier that mapped every 401/403 to auth. Route probe failures through the shared provider-failure authority instead: a Kimi 403 carrying a permission envelope stays neutral rather than demanding re-authentication. The durable diagnostic also prefers the status-bearing cause over an SDK wrapper whose transport code would otherwise shadow the real HTTP status. Generated-by: Maka
Failed turn snapshots now carry the stable failure code and the bounded provider-summary marker from the canonical terminal error fact, and the session projector forwards both to the projected error event. Desktop can therefore render the bounded provider wording without re-deriving meaning from HTTP status or message text. Generated-by: Maka
The error-toast fallback now requires the bounded provider-summary marker before showing a message raw. A bare code is no longer enough, since Node transport codes carry unbounded internal text. Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Codex
Generated-by: Codex
- Require positive provider provenance before certifying a chain link's message as a bounded provider message: a plain-object cause whose only message is its own .message is internal text, never provider wording. - Rank numeric status above free text in classification: a 429/500 whose body or JSON key names mention "aborted" keeps RateLimit / ProviderUnavailable, preserving retry-after handling. - Treat a text-derived Abort as non-retryable, matching the RetryError early return so the same class no longer carries opposite retry semantics. - Select the projected message and its paired code/status from the same cause-chain link instead of combining an inner link's message with an outer link's provider code. - Gate ErrorEvent.code through a closed vocabulary (semantic provider codes, numeric HTTP statuses, Maka-owned sentinels) so a provider's free-form token can never steer the Host's terminal-state taxonomy or the Desktop label/recovery matching. - Use Object.hasOwn for the provider-influenced notice-copy lookup. - Replace the epoch ladder assertion with handshake compatibility coverage and test both older- and newer-peer epoch rejection. Generated with AI assistance Generated-by: Maka
Generated-by: maka
…re authority Main's apache#3365 added ProviderCapacity classification while this branch was queued. Fold it into the unified provider-failure taxonomy: the class joins the core union, structured capacity codes rank above account-state codes, and capacity stays retryable through the shared retry metadata. Generated-by: maka
…class A provider envelope whose nested status contradicts its weak wording (e.g. an "authentication" phrase behind HTTP 500) was classified ProviderUnavailable by the failure projection but re-classified Auth by the retry owner, skipping a transient 5xx. Both paths now share one normalized classification and the same numeric-status resolution. Also surfaces the Runtime-owned provider_capacity reason in the CLI transcript notice map so an empty-text capacity notice keeps the wait/switch-model guidance, and feeds nested data.error codes/status into classification evidence for JSON-shaped errors. Generated-by: maka
db17647 to
b2b2351
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
#2521 b2b2351 — review (bind exact head)
Gate: CI approved runs; verdict independent of gate.
Verdict: NO-GO
[P1] Build artifact in tree — packages/runtime/dist-iso/old-pec.cjs etc — gh api contents on main 404 (not tracked). Debug residuals must be untracked.
[P1] Scope / rollback inseparability — 44 files beyond classification — sampled desktop/runtime-host/CLI layers not required for same defect (cf #3660 +109/-1 minimal). Blast radius.
[P2] Epoch contended — protocol/index.ts bump to 45 contends with #3316/#3376/#3651. Guard is strictly-greater-than base; fix: rebase to current main and take strictly greater.
Coexistence: #3660 is correct minimal shape for same defect; cannot co-exist with #2521 wide form.
Summary
After #2675, Maka already has one safe path for provider diagnostics: ModelAdapter keeps an allowlisted, redacted, bounded summary; the terminal RuntimeEvent remains canonical; Runtime Host projects the message through failed Turn snapshots and recovery; and Desktop can include the exact execution trace in copied reports.
This PR fills the remaining classification gap on top of that plumbing:
context_length_exceededenvelope reaches compaction/recovery instead of being misread as Abort or RateLimit;permission_errorremains unknown and non-retryable, so the bounded provider explanation reaches TUI/Desktop instead of being replaced withAuthentication failed;permission_denied,usage_limit_reached, andinsufficient_quotaproduce distinct non-retryable account meanings;Scope, accurately at the current head:
packages/core: two new fields onErrorEvent/RuntimeEventErrorContentfor the structured provider failure, a newprovider-failuremodule, and its export-map entry.packages/runtime: the classification precedence fix at the shared ModelAdapter seam, provider-failure provenance on connection-effect outcomes, and conformance/classification regression coverage.packages/runtime-host: Turn snapshots and connection effects carry the structured provider failure code and the bounded provider message (protocol, canonical turn snapshot, session projector); the compatibility epoch advances to 29 because older peers cannot safely preserve those fields.apps/desktop: main-process and renderer presentation of the account-limit vs. authentication vs. unknown states, including the allowance/plan/reset recovery guidance, in both locales.packages/cli:pi-transcriptrenders the bounded provider message through a small zh/en copy table.The branch was rebuilt on current
main. Older Runtime Host, Headless, continuity-schema, and diagnostics changes were removed earlier because #2675 now owns those responsibilities.Fixes #2516
Verification
npm --workspace @maka/core test— all passnpm --workspace @maka/storage test— all passnpm --workspace @maka/runtime test— all pass (provider classification, conformance, telemetry)npm --workspace @maka/runtime-host test— all pass (incl. protocol + handshake-compatibility epoch assertions at epoch 29)npm --workspace @maka/cli test— all pass (incl.pi-transcriptzh/en projection tests)apps/desktopmain-process suite — all pass (incl. provider-failure presentation matrix)npm run lint, Biome formatting, andgit diff --check origin/main...HEADI did not repeat a live request against an exhausted Kimi account. The provider-boundary tests replay the observed HTTP 403 envelope and exact message, and the TUI/Desktop tests verify the resulting Runtime Host error message is rendered verbatim.
Checklist
Does this PR entail a change in behavior?
中文说明
#2675 合并后,provider 诊断已经有了一条统一且安全的链路:ModelAdapter 保留经过白名单筛选、脱敏和长度限制的摘要;terminal RuntimeEvent 仍是 canonical fact;Runtime Host 将消息投影到失败 Turn snapshot 和恢复流程;Desktop 复制错误报告时可以附上精确的 execution trace。
本 PR 在这条链路之上补齐剩余的分类缺口:
context_length_exceededenvelope 会进入压缩/恢复流程,而不是被误分为 Abort/RateLimit;permission_error保持为不可重试的 unknown,因此 TUI/Desktop 会显示经过边界处理的 provider 说明,而不是替换成Authentication failed;permission_denied、usage_limit_reached、insufficient_quota等稳定标识会得到不同的、不可重试的账户状态语义;当前 head 的真实改动范围:
packages/core(ErrorEvent/RuntimeEventErrorContent 两个新字段、provider-failure 模块及 export-map 入口)、packages/runtime(ModelAdapter 分类优先级修正、connection-effect provenance、分类/conformance 回归)、packages/runtime-host(Turn snapshot 与 connection effects 携带结构化 provider failure code 和有界 provider 消息;兼容 epoch 推进到 29)、apps/desktop(账户限额/认证/未知三类状态的主进程与渲染层呈现,双语言恢复引导)、packages/cli(pi-transcript 的 zh/en 渲染)。没有再次消耗一个真实的 Kimi 限额账户发请求;provider 边界测试复用了实际观察到的 HTTP 403 envelope 和完整提示,TUI/Desktop 测试验证 Runtime Host 错误消息会原样显示。
Visual evidence
A sanitized structured usage-limit state in Desktop and TUI. The stable provider identifier is presented as an account limit—not authentication—and Desktop gives the corresponding allowance/plan/reset recovery guidance.
AI use
Tool(s) and scope: Maka authored the classification implementation, tests, verification, documentation, and visual evidence. OpenAI Codex authored the review remediation that keeps structured context-length failures out of generic retry (with its regression test) and the epoch-27 compatibility remediation. Maka authored the subsequent review remediations, the rebases that advanced the epoch to 28 and then 29 after main took each preceding value, this PR-body correction to match the actual diff, and the CI flake analysis.
Final squash trailers:
Generated-by: MakaGenerated-by: Codex